Conversation
📝 WalkthroughWalkthroughReplaces "Ongoing" with a v0.47.2 changelog entry, refactors Circle.clock_synchronize() to use an extracted _send_clock_set_req() and early weekday drift check, adds a month-end clock-rollover test, extends test response data, and bumps pyproject version to 0.47.2. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #400 +/- ##
==========================================
+ Coverage 82.05% 82.08% +0.03%
==========================================
Files 36 36
Lines 8156 8181 +25
==========================================
+ Hits 6692 6715 +23
- Misses 1464 1466 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugwise_usb/nodes/circle.py`:
- Around line 885-891: The month overflow handling around calculating
circle_timestamp is wrong: when (difference := days_diff - days_to_end_of_month)
> 0 you adjust corrected_day but do not advance the month, producing incorrect
dates; replace the manual day correction logic (the block using
last_day_of_month, days_to_end_of_month, corrected_day and
dt_now.replace(day=...)) by computing circle_timestamp as dt_now +
timedelta(days=days_diff) (using the existing dt_now and days_diff variables) so
month/year boundaries are handled correctly and remove the fragile
corrected_day/replace logic.
There was a problem hiding this comment.
Pull request overview
Fixes clock synchronization date calculation when the target weekday crosses a month boundary (avoiding invalid datetime.replace(day=...) behavior at month-end).
Changes:
- Update
clock_synchronize()to compute the target date viadt_now + timedelta(days=...)to handle month rollovers safely. - Add an “Ongoing” changelog entry referencing the PR/issue.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
plugwise_usb/nodes/circle.py |
Switches clock-sync date computation to timedelta arithmetic to prevent month-end overflow. |
CHANGELOG.md |
Documents the fix in the ongoing changelog section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_usb.py`:
- Around line 3043-3083: The test_test_clock_synchronize_month_overflow case
doesn't force a month rollover because the frozen date "2026-01-31" is already
Saturday (weekday 5) and mock_clock_get_send sets response.day_of_week.value = 5
so days_diff == 0; update the test so the mocked day_of_week differs by +1 to
force crossing into February (e.g., set response.day_of_week.value = 6) or
change the frozen time in the `@freeze_time` decorator to a Friday date so that
mock_clock_get_send's day_of_week=5 results in a +1 day rollover; modify the
mock_clock_get_send implementation (and/or the freeze_time value) inside
test_clock_synchronize_month_overflow to ensure days_diff != 0 and the code path
that adds timedelta(days=1) is exercised.
dirixmjm
left a comment
There was a problem hiding this comment.
LGTM!
Bedankt voor de actie.
|
@dirixmjm Thanks, but I'm not there yet, trying to recreate the |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@plugwise_usb/nodes/circle.py`:
- Around line 912-913: The docstring for async def _send_clock_set_req currently
incorrectly references "CirclePlusRealTimeClockSetRequest"; update it to
accurately describe that this method sends a CircleClockSetRequest (e.g., "Send
CircleClockSetRequest.") so the docstring matches the implemented behavior in
_send_clock_set_req and avoids confusion with CirclePlus methods.
- Around line 883-888: The early return calls _send_clock_set_req() when
dt_now.weekday() != response.day_of_week.value but the null-check for
self._node_protocols happens later, which can cause an AttributeError; either
move the existing self._node_protocols is None check so it runs before the early
return or add the same guard at the start of _send_clock_set_req() (and
return/raise gracefully) so _send_clock_set_req() never accesses
self._node_protocols.max when _node_protocols is None; update the call site or
the helper accordingly and ensure logging/behavior remains consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
910-923: Helper extraction looks good, but needs the null guard.The refactoring to extract
_send_clock_set_req()follows the pattern established incircle_plus.pyand improves code reuse. The implementation is correct, but as noted above, the method should include the_node_protocolsnull check internally to protect all callers.Once the null guard is added to the helper, the redundant check at lines 906-909 can be removed since both call sites will be protected by the method itself.
|




Summary by CodeRabbit
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.